Fix: combining structural and alt_structural tags#26
Conversation
Original commit from ZW (f262f1d) may have been lost in git history cleanup. - force float conversion of replace_case and sim_rt to ensure no non-numeric items and enhance robustness - ser_qty now is 1D and scalar for edge cases where no component matched leading to len(series) == 0
- Force conversion of simulated_replacement_time to numeric in case of non-numeric cases (same fix as in red_tag.py)
- Fixed a bug to concatenate the two sets of structural systems and subassemblies (e.g. 5=SCBF, 6=SMRF) rather than adding them. i.e. result of structural_systems should be [5,6] and not [11]
- Detects `uid` in meta-tags and throws an error to the user to manually condense duplicate cmp-loc-dir-ds cases. - This case is not encountered if inputs were generated using PBE tool
- Script examples for import usage now follows installation behavior (i.e. no mentioning of path, importing atc138 vs. src.atc138)
dustin-cook
left a comment
dustin-cook
left a comment
There was a problem hiding this comment.
Overall looks good and I think can be merged. Just a few comments/questions that are optional.
| # Account for global red tag cases | ||
| replace_case = np.logical_not(np.isnan(np.array(simulated_replacement_time))) | ||
| sim_rt = np.array(simulated_replacement_time, dtype=float) | ||
| replace_case = ~np.isnan(sim_rt) |
There was a problem hiding this comment.
Okay, so this is the third place where this exact same calc is occuring. Seems like we need to caluclate "replace_case" earlier, in preprocesseing, or as part of the build process.
|
|
||
| # For each structural system | ||
| structural_systems = np.unique(np.array(damage['comp_ds_table']['structural_system'] + damage['comp_ds_table']['structural_system_alt'])) | ||
| structural_systems = np.unique(np.concatenate( |
| sys_qty = np.nanmax(ser_qty, axis = 1) | ||
| sys_ratio = sys_dmg / sys_qty | ||
| sys_tag[:,sys] = sys_ratio > sc_thresholds[sc] | ||
| empty_bin = (ser_dmg.size == 0 or ser_dmg.shape[1] == 0 or ser_qty.size == 0) |
There was a problem hiding this comment.
These changes seem good
| ser_dmg[:, ser] = np.sum(sc_dmg[:, (ser_filt_ds & ss_filt_ds)], axis=1) | ||
|
|
||
| # Total number of components within this series and system (constant across realizations) | ||
| ser_qty[ser] = np.sum(num_comps[ser_filt_comp & ss_filt_comp]) |
There was a problem hiding this comment.
is there a computational benefit to reducing the array dimensions here? I typically like to keep the "meanings" of array dimensions consistent where possible (ie rows = realizations, columns = comps), for more traceable matrix calculations, but that may be more of my matlab bent, and the traditional python approach may be to keep things as dimensionally simple as possilbe.
There was a problem hiding this comment.
Hi Dustin, good point. I think it’s less about computational benefit and more about matching the physical meaning.
ser_dmg varies across realizations, so keeping it 2D (reals × series) makes sense. But ser_qty is just the installed quantity and shouldn't vary with realizations, so I think it's best to keep it 1D.
When it was 2D before, it would cause issues in some models. For example, reductions could be mathematically wrong, and empty selections can cause the analysis to error out.
There was a problem hiding this comment.
I will push back a bit on the physical meaning; the quantity of components can change with realization, if you are uncertain of the quantity (in fact I think PELICUN supports this). However, we just dont vary this in the examples (and probably dont robustly support it downstream). However, if its causing problems, then that is a bigger issue, and I am happy with the change.
|
@hgp297, this is ready to merge when you are ready. The one remaining issue is the "replace_case" where i think we need to do that in a earlier part of the code to not have so much duplicate code. I will leave that up to you to decide if we should fit that into this PR, or do that as a new issue. |
I looked into this and there were several cases where the function (e.g. Since I favor not altering the input arguments, I'm happy keeping the duplicate code to make the mask for now. |
Sounds good |
3 participants
Summary
Fixes the concatenation of
structural_systemandstructural_system_altinred_tag.py, along with previous numeric conversion implemented by Ziyi Wang.Changes:
structural_systemsnow concatenates two arrays rather than adding them. e.g. list of all structural systems with tags [5] and [6] should be [5, 6], not [11]ser_qtykept as 1D as a fixed quantity since it’s not supposed to vary across realizationsinput_builder.pydetects ifuidis present in meta-tag, returns error, and instructs user to manually condense duplicates